refactor: EventLoop locking cleanups + client disconnect exception #160
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Because this change removes --- a/src/ipc/capnp/protocol.cpp
+++ b/src/ipc/capnp/protocol.cpp
@@ -41,10 +41,7 @@ class CapnpProtocol : public Protocol
public:
~CapnpProtocol() noexcept(true)
{
- if (m_loop) {
- std::unique_lock<std::mutex> lock(m_loop->m_mutex);
- m_loop->removeClient(lock);
- }
+ m_loop_ref.reset();
if (m_loop_thread.joinable()) m_loop_thread.join();
assert(!m_loop);
};
@@ -83,10 +80,7 @@ public:
m_loop_thread = std::thread([&] {
util::ThreadRename("capnp-loop");
m_loop.emplace(exe_name, &IpcLogFn, &m_context);
- {
- std::unique_lock<std::mutex> lock(m_loop->m_mutex);
- m_loop->addClient(lock);
- }
+ m_loop_ref.emplace(*m_loop);
promise.set_value();
m_loop->loop();
m_loop.reset();
@@ -96,6 +90,7 @@ public:
Context m_context;
std::thread m_loop_thread;
std::optional<mp::EventLoop> m_loop;
+ std::optional<mp::EventLoopRef> m_loop_ref;
};
} // namespace |
This was referenced Feb 10, 2025
This commit makes mechanical changes needed to simplify an upcoming commit which replaces EventLoop* with an EventLoopRef. This change also happens to be also useful on its own so clientInvoke can detect disconnections in a non-racy way (bitcoin-core#123 (comment)) by seeing if the client Connection pointer is null while holding the event loop mutex.
Use EventLoopRef to avoid reference counting bugs and be more exception safe
Now that they are only called in one place by EventLoopRef class, they can be inlined.
Use kj::Function instead of std::function to avoid the need for AsyncCallable and DestructorCatcher classes, which were used to work around the requirement that std::function objects need to be copyable. kj::Function does not have this requirement. Change is from bitcoin-core#144 (comment) Co-authored-by: Ryan Ofsky <[email protected]>
Add basic thread safety annotations to EventLoop. Use could be expanded to other functions. Use can be expanded and deepened but this puts basic functionality in place. Use of annotations was discussed in bitcoin-core#129 (comment)
Improve clientInvoke exceptions so IPC clients can more reliably detect when they are calling a remote method after the connection is closed. Before this change different exceptions were thrown, which made this condition difficult to detect and handle.
ryanofsky
changed the title
ryanofsky
changed the title
ryanofsky
changed the title
|
Updated ce4814f -> b47ea9f ( |
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 24, 2025
Use EventLoopRef to avoid reference counting bugs and be more exception safe and deal with removal of addClient/removeClient methods in bitcoin-core/libmultiprocess#160 A test update is also required due to bitcoin-core/libmultiprocess#160 to deal with changed reference count semantics. In IpcPipeTest(), it is is now necessary to destroy the client Proxy object instead of just the client Connection object to decrease the event loop reference count and allow the loop to exit so the test does not hang on shutdown.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 24, 2025
This fixes an error reported by Antoine Poinsot <[email protected]> in bitcoin-core/libmultiprocess#123 that does not happen in master, but does happen with bitcoin#10102 applied, where if the child bitcoin-wallet process is killed (either by an external signal or by Ctrl-C as reported in the issue) the bitcoin-node process will not shutdown cleanly after that because chain client flush() calls will fail. This change fixes the problem by handling ipc::Exception errors thrown during the flush() calls, and it relies on the fixes to disconnect detection implemented in bitcoin-core/libmultiprocess#160 to work effectively.
|
Rebased b47ea9f -> f15ef6c ( |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces RAII reference counting to the
EventLoopclass to simplify a recent bugfix in #159. It also deletesDestructorCatcherandAsyncCallablehelper classes as discussed in #144 (comment) and it adds clang thread safety annotations as discussed in #129.This also cleanup up disconnected exceptions to help resolve #123 by letting clients easily detect when they are trying to call a remote method after a connection has been closed. This change is part of the same PR because it depends on adding a
ProxyContext::loopstruct member, which is done in an earlier commit.